Can you explain why the research_event_data is passed around here?

This was in the original code and not introduced here. I think this is a bug and an anti-pattern. We should open a bug to fix this after this PR merges.

The function names are changing unnecessarily across the three cases.

The original was/is:

def reset_course_deadlines(request)

The refactored helper is:

def reset_deadlines_for_course(request, course_key, research_event_data={})
The new build version is:

def reset_all_relative_course_deadlines(request)

I think we should have something like:

def reset_course_deadlines(request)
def reset_course_deadlines(request, course_key, research_event_data={})
def reset_all_course_deadlines(request)

I think it would probably be useful to have the core function of resetting in bulk be:

def reset_bulk_course_deadlines(coursekey_list)

This would make it available without passing in a request.

I think we should have something like:

def reset_course_deadlines(request)
def reset_course_deadlines(request, course_key, research_event_data={})
def reset_all_course_deadlines(request)

We can’t actually have both in the same file, because Python will just override the first with the second.

Did you mean:

• rename the view reset_all_relative_course_deadlines to reset_all_course_deadlines
• rename the util reset_deadlines_for_course to reset_bulk_course_deadlines(coursekey_list)?

Just want to confirm before refactoring, because right now reset_deadlines_for_course is request-aware (getting course_detail). I can leave request in parameters and use coursekey_list for iterating.

Yes, you are right. I think that that naming convention you suggest makes sense. Ideally, I think, we wouldn't pass the request into the utility method because it couples it to the request cycle. However, this code is already messy because we're passing around the research event which is definitely non-standard, probably an anti-pattern.

I made changes in this commit - 163c7d1

Nit: There's no reason for this to be an re_path just use the standard path which.

Changed

Why do we need BearerAuthentication instead of just using the defalut JWT and Session authentication endpoints?

I added it by analogy with a similar existing view (reset_course_deadlines). Additionally, mobile clients use bearer authorization, so BearerAuthenticationAllowInactiveUser is needed to support that alongside the default JWT and session authentication. For now it's used only on mobile application (bearer), but in future it also can be used on web (jwt+session)

This seems like it could take a long time based on the size of the course list. Shouldn't this be a background(celery) task? Have we looked at the performance for 10,100,1000 courses and determined this is sufficiently fast?

This runs pretty fast since it only updates existing schedules with a new start date - there’s no heavy logic or data creation, here is source code of main function which resets deadline:

def reset_self_paced_schedule(user, course_key, use_enrollment_date=False):
    """
    Reset the user's schedule if self-paced.

    It does not create a new schedule, just resets an existing one.
    This is used, for example, when a user requests it or when an enrollment mode changes.

    Arguments:
        user (User)
        course_key (CourseKey or str)
        use_enrollment_date (bool): if False, reset to now, else reset to original enrollment creation date
    """
    with transaction.atomic(savepoint=False):
        try:
            schedule = Schedule.objects.select_related('enrollment', 'enrollment__course').get(
                enrollment__user=user,
                enrollment__course__id=course_key,
                enrollment__course__self_paced=True,
            )
        except Schedule.DoesNotExist:
            return

        if use_enrollment_date:
            new_start_date = schedule.enrollment.created
        else:
            new_start_date = datetime.datetime.now(pytz.utc)

        # Make sure we don't start the clock on the learner's schedule before the course even starts
        new_start_date = max(new_start_date, schedule.enrollment.course.start)

        schedule.start_date = new_start_date
        schedule.save()

Even if a user had a large number of courses, it should still complete quickly because each operation is just a small database update. Also, it’s very unlikely a single user would have hundreds of self-paced courses that all need to be reset.